Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[COW-SDK] - replace SupportedChainId and change xDai > Gnosis Chain #561

Merged
merged 4 commits into from
May 27, 2022

Conversation

W3stside
Copy link
Contributor

Summary

Part of the cow-sdk integration

  • this replaces the SupportedChainId inside constants/chains with the cow-sdk one
  • also changes any references of xdai to gnosis_chain

@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@W3stside W3stside requested a review from alfetopito May 25, 2022 12:25
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working fine. Just 2 small comments

package.json Outdated Show resolved Hide resolved
src/custom/constants/chains/chainsMod.ts Outdated Show resolved Hide resolved
}

const SAFE_BASE_URL = 'https://gnosis-safe.io'
const CHAIN_SHORT_NAME: Partial<Record<number, string>> = {
[ChainId.MAINNET]: 'eth', // https://github.com/ethereum-lists/chains/blob/master/_data/chains/eip155-1.json
[ChainId.RINKEBY]: 'rin', // https://github.com/ethereum-lists/chains/blob/master/_data/chains/eip155-4.json
[ChainId.XDAI]: 'xdai', // https://github.com/ethereum-lists/chains/blob/master/_data/chains/eip155-100.json
[ChainId.GNOSIS_CHAIN]: 'xdai', // https://github.com/ethereum-lists/chains/blob/master/_data/chains/eip155-100.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but this is only internal.
While I do agree with you and this is mildly annoying, I'm fine to have the long version

}
}

// Production, staging, ens, ...
return {
[ChainId.MAINNET]: process.env.REACT_APP_API_URL_PROD_MAINNET || 'https://api.cow.fi/mainnet/api',
[ChainId.RINKEBY]: process.env.REACT_APP_API_URL_PROD_RINKEBY || 'https://api.cow.fi/rinkeby/api',
[ChainId.XDAI]: process.env.REACT_APP_API_URL_PROD_XDAI || 'https://api.cow.fi/xdai/api',
[ChainId.GNOSIS_CHAIN]: process.env.REACT_APP_API_URL_PROD_XDAI || 'https://api.cow.fi/xdai/api',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe update the env variable names for consistency or is it fine to leave it as this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, will create an issue to address it post merge

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@nenadV91 nenadV91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with some minor comments

@alfetopito alfetopito merged commit 5b8bf3e into develop May 27, 2022
@alfetopito alfetopito deleted the cow-sdk-chain-id branch May 27, 2022 13:48
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants